Fix/MultiFile pattern (Issue #696) - #697
Conversation
- Fixed/MultiFile rollover patterns with an index before the extension:
- RolloverFilenameBuilder: the parser was dropping literal text after the final placeholder, including .log.
- *$J(.).log now correctly expands: app.log → app.1.log → app.2.log.
- Updated MultiFile mask dialog legend:
- Fixed incorrectly displayed <date> and <prefix> placeholders in English and German resources.
- Added both usage examples:
*$J(.) → app.log, app.log.1, app.log.2
*$J(.).log → app.log, app.1.log, app.2.log
- Added the examples in English, German, and Chinese.
- Enlarged the dialog and syntax-help label so the date-format lines are no longer clipped.
- Fixed/Items in menu File/Multifile -> "Enable MultiFile" and "Multi file mask...", were always greyed out. Now they are displayed normally after the .log file is open.
- removed the BackColor and ForeColor assignments for both:
- multiFileEnabledStripMenuItem
- multifileMaskToolStripMenuItem
- Added regression coverage:
- Filename-builder tests for app.1.log and app.2.log.
- Reader-level test that loads app.log, app.1.log, and app.2.log in correct order using *$J(.).log.
- Kept the existing *$J(.) coverage for app.log.1 / app.log.2.
Verification:
- Full test suite: 1064 passed, 0 failed, 7 skipped.
- Resource project build: passed with 0 warnings, 0 errors.
| } | ||
| } | ||
|
|
||
| _ = result.Append(Regex.Escape(segment.ToString())); |
There was a problem hiding this comment.
Duplicated Code — [RolloverFilenameBuilder.cs:241] is the exact shape of the state-0 flush at line 202. A tiny local FlushEscaped() would let both share it
There was a problem hiding this comment.
Added FlushEscaped() to share duplicated escaping code.
| } | ||
| } | ||
|
|
||
| _ = result.Append(Regex.Escape(segment.ToString())); |
There was a problem hiding this comment.
Index parsing from a rotated file is still broken. Spec: "app.1.log → Rotated log (Index 1)". RolloverFilenameHandler calls SetFileName on whatever file the user opens; for $J(.).log the regex .(?'index'\d*).log has a greedy .* that swallows the digit in app.1.log, so Index parses as 0 and BuildFileName would produce app.1.1.log. Grouping only works when the active file (app.log) is opened. The same flaw pre-exists for *$J(.) with app.log.1, so it's parity rather than a regression — so the index mapping is only partially delivered.
There was a problem hiding this comment.
- fixed parsing of
$J(<prefix>)from rotated files, so bothapp.log.1andapp.1.logcorrectly identify index 1. - made wildcard matching non-greedy and anchored the generated regex to the complete filename.
- updated rebuilding logic to remove an existing prefix plus index together before inserting the new index.
- fixed $D(...) parsing to use positions in the escaped format string, preventing errors when a literal regex character such as . appears before $D
Added coverage for opening:
- engine.log.1 with
*$J(.)→ builds engine.log.2 - engine.1.log with
*$J(.).log→ builds engine.2.log
| [TestCase("engine1.log", "engine2.log","engine$J.log")] | ||
| [TestCase("engine.log", "engine.log.1","*$J(.)")] | ||
| [TestCase("engine.log", "engine.1.log", "*$J(.).log")] | ||
| [TestCase("engine_2010-06-12.log", "engine_2010-06-12.log.1", "*$D(yyyy-MM-dd).log$J(.)")] |
There was a problem hiding this comment.
$I untested. For "$J, $I" support, the fix itself generalizes, but no test covers $I or $D followed by a trailing literal (e.g. *$D(yyyy-MM-dd)$J(.).log).
There was a problem hiding this comment.
Added cases which cover:
- $I with a trailing literal .log, starting from a rotated file.
- $D plus $J(.) followed by a trailing .log, also starting from a rotated file.
| [TestCase("engine1.log", "engine2.log","engine$J.log")] | ||
| [TestCase("engine.log", "engine.log.1","*$J(.)")] | ||
| [TestCase("engine.log", "engine.1.log", "*$J(.).log")] | ||
| [TestCase("engine_2010-06-12.log", "engine_2010-06-12.log.1", "*$D(yyyy-MM-dd).log$J(.)")] |
There was a problem hiding this comment.
ParseFormatString computes datePos from the original string but edits the escaped one — a literal with regex metachars before $D (e.g. app.$D(yyyy-MM-dd)) corrupts the regex. Not introduced here, but the new trailing-literal support makes such patterns likelier.
There was a problem hiding this comment.
Fixed by locating and parsing $D(...) in the escaped format string. Offsets remain correct when literal regex metacharacters appear before the date placeholder.
Added a regression test for app.$D(yyyy-MM-dd).log that verifies date parsing and rebuilding.
Fixed by locating and parsing $D(...) in the escaped format string. Offsets remain correct when literal regex metacharacters appear before the date placeholder. Added a regression test for app.$D(yyyy-MM-dd).log that verifies date parsing and rebuilding. Added cases which cover: - $I with a trailing literal .log, starting from a rotated file. - $D plus $J(.) followed by a trailing .log, also starting from a rotated file.
- Added FlushEscaped() to share duplicated escaping code.
- Fixed parsing of $J(<prefix>) from rotated files, so both:
- app.log.1
- app.1.log
correctly identify index 1.
- Made wildcard matching non-greedy and anchored the generated regex to the complete filename.
- Updated rebuilding logic to remove an existing prefix plus index together before inserting the new index.
- Fixed $D(...) parsing to use positions in the escaped format string, preventing errors when a literal regex character such as . appears before $D.
|
Check please |
…mid-filename-index-tags-(app.1.log)
| { | ||
| _ = result.Append(Regex.Escape(segment.ToString())); | ||
| segment = new StringBuilder(); | ||
| } |
There was a problem hiding this comment.
extract it to a real function not a function in a function, this only leads to unreadable code
| StringBuilder result = new(); | ||
| StringBuilder segment = new(); | ||
|
|
||
| void FlushEscaped () |
There was a problem hiding this comment.
FlushEscaped() covers only case 0, while case 1 and case 3 still inline _ = result.Append(segment); segment = new StringBuilder();.
| if (_indexGroup != null && _indexGroup.Success) | ||
| { | ||
| fileName = fileName.Remove(_indexGroup.Index, _indexGroup.Length); | ||
| var indexPosition = _indexGroup.Index; |
There was a problem hiding this comment.
Hidden invariant / Feature Envy — indexPosition = _condGroup.Index; indexLength += _condGroup.Length; silently assumes cond is contiguous and immediately precedes index — an invariant set 60 lines away in ParseFormatString. Uncommented; changing either regex breaks the other.
| } | ||
|
|
||
| [Test] | ||
| public void BuildFileName_DatePatternAfterRegexMetacharacter_IncrementsDate () |
There was a problem hiding this comment.
BuildFileName_X_Y while the file's existing methods are TestFilenameAnd1/2.
| _hideZeroIndex = fmt.Contains("$J", StringComparison.Ordinal); | ||
| fmt = fmt.Replace("$I", "(?'index'[\\d]+)", StringComparison.Ordinal); | ||
| fmt = fmt.Replace("$J", "(?'index'[\\d]*)", StringComparison.Ordinal); | ||
| var optionalIndexPattern = _condContent != null |
There was a problem hiding this comment.
optionalIndexPattern reads as "the pattern is optional";
|
|
||
| File.Copy(Path.Combine(_testDataDirectory, "app.log"), _logFile); | ||
| File.Copy(Path.Combine(_testDataDirectory, "app.log.1"), _logFile + ".1"); | ||
| File.Copy(Path.Combine(_testDataDirectory, "app.1.log"), Path.Combine(_testDirectory, "app.1.log")); |
There was a problem hiding this comment.
SetUp copies app.1.log/app.2.log unconditionally, so all four tests now see two extra files though only the new one needs them.
| fmt = fmt.Replace("*", ".*", StringComparison.Ordinal); | ||
| fmt = fmt.Replace("*", ".*?", StringComparison.Ordinal); | ||
| _hideZeroIndex = fmt.Contains("$J", StringComparison.Ordinal); | ||
| fmt = fmt.Replace("$I", "(?'index'[\\d]+)", StringComparison.Ordinal); |
There was a problem hiding this comment.
"Allow index tags ($J, $I) to appear before a fixed file extension suffix." RolloverFilenameBuilder.cs:189 still maps $I → (?'index'[\d]+); only $J gets the prefix-aware alternation (:190-193). So *$I.log cannot match the active file app.log — SetFileName fails, IsIndexPattern stays false, no chain is built. The one new $I test (RollingNameTest.cs:36, engine1.log→engine2.log) starts from a numbered file and never exercises the failing case.
| $D(&lt;date&gt;) = Datumsmuster | ||
| $D(<date>) = Datumsmuster | ||
| $I = Dateiindexnummer | ||
| $J = Dateiindexnummer, versteckt wenn 09 |
There was a problem hiding this comment.
still reads $J = Dateiindexnummer, versteckt wenn 09 — should be 0. Correct in the English and Chinese files.
| } | ||
|
|
||
| fmt = fmt.Replace("*", ".*", StringComparison.Ordinal); | ||
| fmt = fmt.Replace("*", ".*?", StringComparison.Ordinal); |
There was a problem hiding this comment.
.→.? and :194 \A…\z anchoring change every mask, not just ones with trailing literals. It holds because RolloverFilenameHandler.cs:39 passes a bare filename, not a path — but that invariant is undocumented and untested; a full path with any non-*-prefixed mask now fails where it previously matched.
| @@ -116,15 +116,23 @@ public string BuildFileName () | |||
|
|
|||
There was a problem hiding this comment.
A saved mask *$J(.).log previously behaved as *$J(.) and matched app.log.1; it now means app.1.log. That's what the spec wants, but existing user settings change
This is a fix for #696
Fixed/MultiFile rollover patterns with an index before the extension:
*$J(.).lognow correctly expands: app.log → app.1.log → app.2.log.Updated MultiFile mask dialog legend:
<date>and<prefix>placeholders in English and German resources.*$J(.)→ app.log, app.log.1, app.log.2*$J(.).log→ app.log, app.1.log, app.2.logBefore

After

Before

After

*$J(.).log.*$J(.)coverage for app.log.1 / app.log.2.Verification:
- Full test suite: 1064 passed, 0 failed, 7 skipped.
- Resource project build: passed with 0 warnings, 0 errors.